Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] - Update exceptions item UI #135255

Merged
merged 24 commits into from
Jul 8, 2022

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Jun 27, 2022

Summary

Addresses #135254

Updates the exceptions viewer item UI to match new designs. There are still some small aspects missing, but bulk of updated UI is there.

NOTE: There seems to be an existing bug where the field is not populated when selecting to edit an exception item.

Left to do

  • Add meta info showing what list the item is linked to
  • Add meta info showing what rule the item is linked to

Existing UI

Screen Shot 2022-07-07 at 2 08 39 PM

Screen Shot 2022-06-27 at 1 40 18 PM

Checklist

@yctercero yctercero self-assigned this Jun 27, 2022
@yctercero yctercero added release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team ci:deploy-cloud v8.4.0 labels Jun 27, 2022
@@ -81,21 +78,13 @@ const ExceptionsViewerItemsComponent: React.FC<ExceptionsViewerItemsProps> = ({
exceptions.length > 0 &&
exceptions.map((exception, index) => (
<MyFlexItem data-test-subj="exceptionItemContainer" grow={false} key={exception.id}>
{index !== 0 ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removes the OR between exception item cards as it no longer appears in new designs.

@yctercero yctercero marked this pull request as ready for review June 29, 2022 04:07
@yctercero yctercero requested review from a team as code owners June 29, 2022 04:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@yctercero yctercero requested a review from yiyangliu9286 June 29, 2022 04:08
<div data-test-subj={dataTestSubj}>
{entries.map((entry, index) => {
const { field, type } = entry;
const value = 'value' in entry ? entry.value : '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, do we have here any specific reason to use the in operator and not something like:
const value = entry?.value ?? ''

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funky because of the typescript union in which a specific property (in this case value) is not present in all union types, so ? doesn't solve it in this case. I did try, but typescript still yelled at me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you're right!

Found this issue

}, [exceptionItem.comments]);

const disableItemActions = useMemo((): boolean => {
const foundItems = loadingItemIds.filter(({ id }) => id === exceptionItem.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Because later you check for foundItems.length > 0 maybe better here to use .some which will return you boolean in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating!

fieldName="created_by"
label={i18n.EXCEPTION_ITEM_CREATED_LABEL}
value1={
<FormattedDate fieldName="created_by" value={item.created_at} dateFormat="MM/DD/YY" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we specifically use this format here? As I understand it's US only.)

Also in Kibana Advanced settings, we have Date format - the user can specify how they want present dates, maybe it's better to use this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point. I updated to not specify so that it should fall back to the users date preference.

@nkhristinin
Copy link
Contributor

  1. Here is text alignment is a bit off:

Screenshot 2022-06-30 at 11 18 40

  1. After I created a new rule and add an exception, the exception list blinked for a second and after that didn't show until I click refresh:
Screen.Recording.2022-06-30.at.12.05.29.mov

Copy link

@yiyangliu9286 yiyangliu9286 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the component usage we use in the designs for Created and Updated is EuiBadges instead of EuiBetaBadge just to be consistent with the designs.
Screen Shot 2022-06-30 at 10 57 04 AM

@yctercero
Copy link
Contributor Author

yctercero commented Jul 7, 2022

@nkhristinin @yiyangliu9286

Updated the UI to:

  • make sure the text around Created and Updated aligns
  • use EuiBadge instead of EuiBadgeBeta
  • remove the WHEN on an initial condition since it's not in the design, designs just have no descriptor word for the initial condition
  • Allow date to be formatted per user's advanced settings preference

Also, here's an example of what an endpoint exception item looks like.

Screen Shot 2022-07-07 at 2 08 39 PM

@yctercero
Copy link
Contributor Author

  1. Here is text alignment is a bit off:
Screenshot 2022-06-30 at 11 18 40
  1. After I created a new rule and add an exception, the exception list blinked for a second and after that didn't show until I click refresh:

Screen.Recording.2022-06-30.at.12.05.29.mov

Addressed both issues - thanks for finding this @nkhristinin !

bug_fix.mov

@nkhristinin nkhristinin self-requested a review July 8, 2022 13:19
Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exceptions card looks pleasant now, great work! LGTM!

@yctercero yctercero enabled auto-merge (squash) July 8, 2022 19:42
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jul 8, 2022

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3112 3113 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.2MB 5.2MB +46.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 442 441 -1

Total ESLint disabled count

id before after diff
securitySolution 517 516 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @yctercero

@yctercero yctercero merged commit 0a0b0a1 into elastic:main Jul 8, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 8, 2022
@yctercero yctercero deleted the entry_ux branch July 8, 2022 21:17
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants